Repository navigation
(hardening) Protect QuickTree add and clear actions against CSRF - #42
somethingwithproof wants to merge 6 commits into
Conversation
c0e3160 to
d43e106
Compare
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
d43e106 to
cbb856b
Compare
TheWitness
left a comment
TheWitness
left a comment
There was a problem hiding this comment.
We keep doing this differently. I'm not entirely happy with the way it's done here.
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
778a07d routes the add and clear checks through one named guard in |
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Resolved the conflict with main in 6ed16a1, preserving both the named CSRF guard and the newly merged tree-authorization helper. The 23 mutation cases, four authorization cases and JavaScript POST test pass locally; hosted PHP 8.2, 8.3 and 8.4 integration and Sonar checks also pass. The requested consistency review remains open for your decision. |
| } | ||
|
|
||
| // remove stays GET until its row links become POST forms. | ||
| if (in_array($action, array('add', 'clear', 'save'), true)) { |
There was a problem hiding this comment.
@TheWitness Which pattern do you want plugins standardised on? Core now has csrf_require_post() in include/csrf.php on 1.2.x and develop (unreleased, and only the develop version also calls csrf_check(false)), while plugin_pstat#9 and plugin_audit inline the POST check plus csrf_check(false), so I can either call csrf_require_post() with a function_exists() fallback for compat 1.2.29 or inline the pstat block and drop quicktree_mutation_status().
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
This change protects QuickTree’s add, clear and save requests against cross-site request forgery. The server validates the effective action, requires a POST with a valid Cacti token, and the graph-add client uses the existing CSRF-aware AJAX transport. This prevents a simple link or untrusted page from triggering those actions; row removal remains outside this change.
Implementation details
Adding or clearing queued graphs currently changes state through GET, including the bulk-action clear alias. Validate the effective action after remapping, require POST and a successful Cacti CSRF check, and send graph additions as JSON-response AJAX POST requests through the host CSRF-aware transport. The existing bulk clear form already submits by POST.
Validation: PHP 8.4.25 standalone regression, 16 direct/alias request cases passed; Node 22.22.2 client regression passed, including preserved graph fields and notifications; syntax and diff checks passed.
Closes #40.
Revision and checks
Description scope: target
main, 7 changed files, head6ed16a1d4. Checked 2026-10-23 03:06:04 UTC.GitHub check snapshot: 7 passed. Local test figures and earlier CI runs above are recorded evidence for their stated revisions and environments; they do not assert that every check passes at this head.